Repository navigation
Conversation
PR governanceThis PR follows the template and is marked ready for human review. |
| report = savings_ledger.aggregate_savings( | ||
| retention_days=days, group_by=group_by, where=where_filter | ||
| ) | ||
| routing = _proxy_routing() |
There was a problem hiding this comment.
🟡 Filtered savings include global routing totals
With --where host=studio, savings prints studio's ledger totals beside unfiltered routing totals. _proxy_routing reads the proxy-wide rollup, so routing savings appear attributed to studio.
Learn more
The ledger applies where_filter before computing any savings totals, but _proxy_routing reads /stats.routing without a corresponding label filter. The CLI appends that global rollup to both JSON and human-readable filtered output, even when the selected label has no matching ledger events.
Example: A studio request saved $1 and another host's routed requests saved $20. headroom savings --where host=studio displays studio's $1 ledger result alongside $20 in model routing, although those routing requests were not from studio.
Recommended fix: Suppress routing when a --where filter is active, unless the routing rollup gains a compatible per-label filter. Keep unfiltered routing output unchanged.
Was this helpful? React with 👍 or 👎 to provide feedback.
| {"project": project} if project else None, | ||
| {"client": outcome.client} if outcome.client else None, |
There was a problem hiding this comment.
🟡 Disabled or replaced labels still get recorded
When client is disabled or given a custom resolver, emit_request_outcome records the handler's client instead. The legacy fields override resolved labels, so configured exclusions and custom identities cannot control those dimensions.
Learn more
Request middleware resolves only the configured dimensions and stores the results in the request context. The outcome funnel then adds outcome.project and outcome.client unconditionally to that mapping. Consequently the dimension configuration cannot remove those names, and a registered resolver for either name loses to the handler's legacy field. The project also remains in the existing project map for backward compatibility; the new by_dimension map needs to distinguish that compatibility behavior from enabled attribution.
Example: With HEADROOM_ATTRIBUTION_DIMENSIONS=default,-client, a request from Codex has no middleware client label. Its RequestOutcome.client='codex' adds client=codex anyway, and /stats-lifetime.by_dimension.client reports it.
Recommended fix: Gate adding legacy project and client to the new attribution mapping on enabled dimensions, and do not overwrite values supplied by a registered resolver. Preserve the existing legacy project and client bookkeeping separately.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for name, label in parse_labels(request.header(LABELS_HEADER)).items(): | ||
| if len(labels) >= MAX_DIMENSIONS: | ||
| break | ||
| if name not in labels and name not in registry and not _is_credential(name): |
There was a problem hiding this comment.
🟡 Open labels omit registered dimensions
With only labels enabled, resolve_attribution drops combined-header labels whose names are registered dimensions. The registry check excludes names even when their resolver was not selected, leaving those requests unattributed.
Learn more
The labels configuration token reads client-named dimensions from X-Headroom-Labels. A registered dimension that is not in names has not run its resolver, but the open-label loop still discards its supplied value solely because it exists in registry.
Example: An extension registers team with default=False; the operator sets HEADROOM_ATTRIBUTION_DIMENSIONS=labels, and a client sends X-Headroom-Labels: team=infra. resolve_attribution returns no team label rather than team=infra.
Recommended fix: Let open labels supply registered names that were not resolved through the selected dimensions, while preserving the precedence of explicitly selected resolvers and credential checks.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| def format_labels(labels: Mapping[str, str]) -> str: | ||
| """Inverse of :func:`parse_labels`, for building ``X-Headroom-Labels``.""" | ||
| return "; ".join(f"{k}={v}" for k, v in normalize_attribution(labels).items()) |
There was a problem hiding this comment.
🟡 Label formatting loses values with separators
When a label contains a comma or semicolon, format_labels emits an ambiguous header value. parse_labels splits it, so a round trip changes the label.
Learn more
The new formatter advertises an inverse of parse_labels, which separates pairs on commas and semicolons. Label sanitation permits both characters, and the formatter does not escape them. A client using this exported helper therefore cannot faithfully send some valid labels through the combined header.
Example: format_labels({'team': 'design, ops'}) emits team=design, ops. Parsing that value yields {'team': 'design'} instead of the original label.
Recommended fix: Define an escaping convention shared by format_labels and parse_labels, or reject separator characters from labels before formatting and document that restriction.
Was this helpful? React with 👍 or 👎 to provide feedback.
| No labelled requests yet. <span class="font-mono">headroom wrap</span> sends | ||
| <span class="font-mono">X-Headroom-Host</span>; add your own with | ||
| <span class="font-mono">HEADROOM_LABELS="team=infra"</span>. |
There was a problem hiding this comment.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
JerrettDavis
left a comment
There was a problem hiding this comment.
The follow-up at 5ad4dc944c83b71a9cf55d376cbf58d9f8390658 addresses the filtered-routing totals and disabled-client booking findings. Two remaining configuration behaviors need resolution before approval:
-
A replacement resolver is lost at the outcome funnel. Registering
clientwithregister_dimension(..., replace=True)and resolving it producescustom-client. Passing that resolved attribution into a real proxy outcome with legacyRequestOutcome.client='codex'then records onlycodexin/stats.savings.by_dimension.client.emit_request_outcomeunconditionally overlays the enabled legacy client/project fields after resolved labels. Preserve an explicitly resolved replacement value in the new attribution map, keeping legacy bookkeeping separate, or explicitly reject/document replacement for these reserved dimensions. Please cover the end-to-end outcome/stats behavior rather than only resolver output. -
The open-label contract excludes registered but unselected names.
resolve_attribution(AttributionRequest(headers={'x-headroom-labels':'project=web'}), ['labels'])returns{}. The documentation sayslabelsaccepts any dimension a client names; the registry exclusion also applies when that registered resolver was not selected. Distinguish selected resolvers from registered-but-unselected names, or explicitly document and test those names as reserved. A selected resolver should still retain precedence over a client header.
Both cases reproduce on the exact published head in owned probes. The source includes current main. These are attribution correctness/contract issues, not CI failures. No contributor logic was rewritten; the broader 18-file feature is not claimed fully qualified by these probes. Maintainers own CI; no contributor CI action requested. Nothing merged.
5ad4dc9 to
eac9e69
Compare
|
@JerrettDavis thanks for the review. Both points are fixed in the latest commit, and the branch is rebased now that #4698 has merged (it no longer carries that PR's commits).
Smaller things fixed along the way: |
|
Two new review points on the current head: (1) aggregate_savings(..., group_by=["calls"]) loses a calls=infra label because _ranked merges the numeric calls field over it; please choose a collision-free row representation or reject conflicting dimension names and cover this case. (2) Please reconcile the replacement-client ledger guarantee: docs promise the resolved label in dims, but _event_dims drops it and the new test asserts that absence. Keeping the legacy client key is understood; the question is whether the resolved label is also retained or the guarantee narrowed. Isolated probes in /job/output/review_probes.py reproduce both; full pytest could not start because pytest is absent. No live proxy behavior claimed. |
|
@JerrettDavis thanks, both are addressed in the new commit (a plain fast-forward on top of the head you reviewed). It also contains a set of other fixes, all listed below so nothing in the diff is a surprise. Your two points
Everything else in the commitPrompted by those two, I went through the rest of the contract the same way and fixed what did not hold. Access
Operator switches
Label handling
Bounds
Smaller
Docs (
Not coveredThe proxy tests hand the labels the middleware bound to |
|
A remaining formatter case changes attribution labels: |
JerrettDavis
left a comment
There was a problem hiding this comment.
Review completed; approval remains pending the items below.
Reviewed head d13f4b925bcc59b1a94a95b84cb8819a107fc932 against main c05fbdddc873078dd97cb9ede841e75ef7ed0bef.
Minor — headroom/proxy/attribution.py:417
format_labels silently changes valid labels containing percent escapes when its output is parsed. Choose an explicit encoding or rejection policy and add a round-trip regression test.
Qualification still required:
- Passing test evidence is missing
- Exact-source provenance review is incomplete
- Check/run 131027444949 is not successful or baseline-qualified
- Check/run 131027444646 is not successful or baseline-qualified
- Check/run 43172051543 is not successful or baseline-qualified
|
@JerrettDavis thanks, fixed in the new commit (a plain fast-forward on top of Percent escapes in
|
Sync PR headroomlabs-ai#4699 with latest main.
Sync PR headroomlabs-ai#4699 with latest main.
JerrettDavis
left a comment
There was a problem hiding this comment.
The percent-escape fix handles the earlier examples, but encoding can now exceed the parser's header limit and silently change attribution. Reproduce with labels = {f"d{i}": "%" * 128 for i in range(16)} followed by parse_labels(format_labels(labels)). All 16 labels survive normalization; expected the same mapping back. Actual: the formatter emits 6,228 characters, the parser reads 4,096, only 11 labels remain, and d10 shrinks from 128 characters to 68. Please reconcile the encoded-size and parser limits, or explicitly reject oversized formatter output, and add a boundary regression. The normal-package regression fails on this head. Existing attribution tests passed across two runs: 69 normally and 12 proxy tests in supported Python-only mode; native compression and live provider traffic were not tested.
Also refuse dimension names starting with - or ., and the label unknown, which the ledger shows for an event with no label.
|
@JerrettDavis thanks, good catch. Fixed, together with a few more things we found reviewing the rest of the PR ourselves. Everything is a fast-forward: a merge of The header limit
LABELS_HEADER_MAX_LENGTH = MAX_DIMENSIONS * (DIMENSION_NAME_MAX_LENGTH + len("=") + 3 * LABEL_MAX_LENGTH) + (MAX_DIMENSIONS - 1) * len("; ")
# 16 * (32 + 1 + 384) + 15 * 2 = 6702That is the longest header The parser also stops shortening labels. When a header is longer than the limit, it is cut back to the last separator, so a pair cut by the limit is dropped instead of being booked under a shortened label (your
Tests (
Other fixes from our own review
Merge with upstream
Test evidenceEverything ran in a sealed container (Python 3.13, no network, sources copied in, nothing mounted), on the tree of the new head:
|
Description
Every figure the proxy reports is a sum over requests, and today only
projectcan split it. Each extra split would need its own header, context variable, outcome field, tracker map, ledger key and dashboard table. This PR adds those pieces once, generically, as proposed in #4676. A request carries a{dimension: label}mapping, and every surface that splits by project can now split by any dimension.headroom/proxy/attribution.py: a dimension is a name plus a resolver. Built-ins areproject(existing header and/p/<name>prefix),client(X-Client, else the User-Agent) andtenant(the tenant-key header). Middleware resolves the enabled dimensions once per request into a context variable.HEADROOM_ATTRIBUTION_DIMENSIONSpicks which ones (default,-name,none,labels). A name with no resolver is read fromX-Headroom-<name>, soteamorcost-centerneeds no code.X-Headroom-Labels: team=infra; cost-center=42sends several at once. With thelabelstoken a client may name any dimension that way, including a built-in that is not listed; a listed resolver always has the last word, and-namekeeps a name out.register_dimension()lets an extension or a deployment's identity layer add its own, andreplace=Truetakes over a built-in. An extension's dimension is reserved fromlabelsunless it registers withclient_settable=True.emit_request_outcomemerges the middleware labels, any the handler knew, and the existingproject/clientfields. The existing fields win their own dimension only while it is enabled and still resolved by its built-in: a dimension turned off stays out, and a replaced resolver keeps the label it gave. It hands the result to the savings tracker, the ledger, the request log tags andOutcomeSnapshot.attribution(PR 1).dimensionsmap per label, with the same accounting and eviction asprojects: 16 dimensions, 50 labels each, the smallest and oldest label evicted first.projectis booked there like any other dimension.projectsis unchanged, so existing readers keep working.dims.headroom savings --where dim=labelfilters every window, and--by <dim>splits the lookback window. An event with no label for a dimension shows asunknown. A dimension named like a report figure (calls,tokens_saved, …) cannot be grouped by and--byrefuses it./stats,/stats-lifetime, dashboard:by_dimensiontotals and a Savings by Label table. Labels can name machines and people, so these fields (savings.by_dimension,persistent_savings.by_dimensionand/stats-lifetimeby_dimension) are returned only to callers that_request_can_view_stats_metadataadmits: the same gate asrecent_requests, including proxy-token holders since feat(proxy): show per-request /stats metadata to token-authenticated callers #4168.security-model.mdxlists the new fields in its token table.HEADROOM_OTEL_ATTRIBUTION=project,team(orall) adds labels as datapoint attributes. Off by default, since every label value becomes a series.Labels are self-reported. They attribute usage and authorize nothing. Authenticated identity stays behind
set_identity_resolver.This is PR 2 of the split proposed in #4676. It builds on #4698, which has merged; the branch is rebased onto main and carries only this PR's commits.
hostandtraffic_classfollow in PR 3.Type of Change
Changes Made
headroom/proxy/attribution.py,attribution_builtins.py(new): dimensions, resolvers, sanitizing, context, OTel provider.headroom/proxy/server.py: resolve labels where project attribution is already captured (HTTP and WebSocket);by_dimensionin/statsand/stats-lifetimebehind the existing metadata gate.headroom/proxy/outcome.py,handlers/openai.py,prometheus_metrics.py: carry the labels to the tracker and ledger.headroom/proxy/savings_tracker.py: per-dimension maps, persisted and normalized on load.headroom/savings_ledger.py,headroom/cli/savings.py:dimson events;--by,--where.headroom/dashboard/templates/dashboard.html: Savings by Label.headroom/pipeline.py: theOutcomeSnapshot.attributioncomment now points to the new module.docs/content/docs/attribution.mdx(new),meta.json, andsecurity-model.mdx(token table).tests/test_attribution.py,tests/test_attribution_savings.py.Testing
pytest)ruff check .)mypy headroom)Test Output
Real Behavior Proof
headroom savings --by clientover the durable ledger.headroom savings --by clientsplits the 30-day window by harness:Runtime Rollout Safety
project,client,tenant) are resolved and booked, and/statsand/stats-lifetimegainby_dimensionfor metadata-admitted callers only. Existing fields andprojectsare unchanged. The OTel attributes and thelabelstoken are off by default.HEADROOM_ATTRIBUTION_DIMENSIONS=nonedimensionsstate is ignored by older versions.Review Readiness
Checklist
CHANGELOG.md— it is generated by release-please from my Conventional Commit PR title (a CI guard enforces this)Screenshots (if applicable)
N/A
Additional Notes
replace=Trueforclientorprojectis no longer overridden at the outcome funnel by the handler's ownRequestOutcome.client/project, nor by the WebSocket and Codex handlers binding those labels. Its label reachesby_dimension, the ledger'sdims, the request-log tags andOutcomeSnapshot.attribution.test_a_replaced_resolver_is_not_overridden_by_the_legacy_fieldtakes the labels the middleware bound on a real request and checks/stats,/stats-lifetime, the log tags and the ledger event. Legacy bookkeeping is separate and unchanged: theprojectstotals and the ledger event's ownclientkey (whichheadroom savings --by clientreads) still record what the proxy detected.by_dimension.projectis now booked from the request's labels like every other dimension. It used to be served fromprojects, which could not show a replaced or client-named project and ignored-project. It therefore starts counting from this version;projectskeeps its history.labelsaccepts a registered dimension that is not selected (resolve_attribution(..., ['labels'])withproject=webreturns it). A selected resolver keeps precedence over the header, even when it resolves nothing, and-nameis never re-admitted, by a client or bybind_label.client_settable=True, so a label taken from authenticated identity cannot be supplied in a header when the operator has not listed it.unregister_dimensionrestores a replaced built-in.default,noneandlabelsare rejected as dimension names.X-Headroom-Labelsis filtered before the 16-label cap applies.aggregate_savingsrefuses agroup_byname that is also a row field (ROW_FIELDS), since{<dimension>: <label>, **figures}cannot hold both; the CLI reports it as a--byerror.--whereon such a name still works.clientkey is always the harness anddimsnever carriesclient; every other dimension is written as resolved./statsno longer returnspersistent_savings.by_dimensionto callers outside the metadata gate (it stripped onlysavings.by_dimension).RequestOutcome.attributionobey the operator's switches. A header label is percent-decoded once. A new dimension can evict the smallest idle one; merged labels are capped at 16; only the first 4 KB ofX-Headroom-Labelsis read.default,noneandlabelscannot become dimensions by any path.client/projecttags follow the label only for a dimension the operator listed.headroom savings: a name that is not a dimension name, or a repeated--wheredimension, is an error; a filter that matches nothing says so;--by clientprints one client section.emit_request_outcome.projectsis kept alongsideby_dimension.projectso no existing reader breaks. Folding it in can be a later change.extract_tagsalready turns everyX-Headroom-*header into a request-log tag. Tags stay as they are, per request, unbounded and never aggregated. A dimension is an operator-chosen, capped subset of those same headers that gets booked into the tracker and ledger.X-Headroom-Team: infrais therefore both a tag and, onceteamis listed, a label, under the same credential rule (is_credential_header).is_credential_headeralready treats as a credential (authorization,cookie, API-key headers, the proxy token), so a dimension name cannot turn a secret into a persisted label.